Skip to content

refactor(billing): security_invoker views + RLS on base tables#709

Merged
softmarshmallow merged 2 commits into
mainfrom
refactor/billing-security-invoker-views
May 7, 2026
Merged

refactor(billing): security_invoker views + RLS on base tables#709
softmarshmallow merged 2 commits into
mainfrom
refactor/billing-security-invoker-views

Conversation

@softmarshmallow

@softmarshmallow softmarshmallow commented May 7, 2026

Copy link
Copy Markdown
Member

Summary

Converts grida_billing's API surface from SECURITY DEFINER views over a locked-down schema → security_invoker = true views over RLS-protected base tables. Same row sets for every legitimate consumer; silences splinter lint 0010 (security_definer_view) that the Dashboard's Database Advisors page was flagging.

This is the canonical Supabase pattern — protection lives in table-level RLS policies that the rest of the codebase already audits the same way, instead of in a SECURITY DEFINER view body that's easy to read past.

What landed

New migration supabase/migrations/20260507223000_grida_billing_security_invoker.sql. The pushed 20260506132900_grida_billing.sql is unchanged.

Surface Before After
grida_billing schema no USAGE for anon/authenticated USAGE granted to authenticated + service_role; revoked from PUBLIC. Required because security_invoker views resolve grida_billing.* as the caller.
account / subscription / audit RESTRICTIVE deny-all on both roles SELECT-only permissive policy (member_can_select / owner_can_select) + table-level GRANT SELECT to authenticated. Writes still service-role only.
product_catalogue / stripe_event locked unchanged — no API consumer reads them
v_billing_subscription security_invoker = false, WHERE filter on auth.uid() security_invoker = true, body keeps only WHERE status <> 'canceled' (auth scoping moved to RLS)
v_billing_audit same pattern same pattern, owner-only filter via RLS
Both views' grants implicit SELECT to anon via Supabase default privileges explicit REVOKE FROM PUBLIC, anon, authenticated then GRANT SELECT TO authenticated, service_role

Behavioral equivalence

Verified — see test plan. Quick summary:

  • Authenticated org member: reads same rows (auth filter just moved from view body to base-table RLS policy).
  • Random authenticated user without membership: 0 rows, before and after.
  • Owner reading v_billing_audit: same rows; non-owner members still blocked.
  • anon querying the views directly: previously got an empty array via auth.uid() = NULL filter; now gets permission_denied because we explicitly revoked. Strictly better posture; no legitimate flow hits this path (all 4 callsites use user-authed createClient()).

Tests

supabase/tests/test_grida_billing_test.sql §13 RLS contract rewritten for the new model:

  • 10× anon-denial assertions (SELECT + INSERT × 5 internal tables)
  • 3× authenticated-without-org-sees-zero on member tables
  • 2× authenticated-denied on locked tables (product_catalogue, stripe_event)
  • 5× authenticated-INSERT-denied across all 5 tables
  • Plan stays at 57 total; all pass.
Gate Result
pnpm --filter editor typecheck ✅ clean
pnpm lint (oxlint) ✅ 0 errors
Editor unit tests (476) ✅ pass
supabase db reset (both migrations apply) ✅ clean
supabase test db (135 pgTAP, +20 net) ✅ pass
BILLING_E2E=1 suite (real Stripe sandbox) ✅ 3/3 — lifecycle, idempotency, tampered-signature
just lint-supabase-db no security_definer_view findings

Tooling

New just lint-supabase-db recipe runs Supabase Splinter advisors against the local DB — same lints the Dashboard's Database Advisors page shows. The CLI doesn't yet ship a native command for this; inline curl + psql + awk is enough.

Recipe links the upstream tracking issue and our comment, so future maintainers can drop it when the CLI ships native support: supabase/cli#3839 (our comment).

Test plan

  • Run supabase db reset against a fresh local supabase
  • Run supabase test db — 135 tests pass, including the rewritten §13
  • Run BILLING_E2E=1 pnpm --filter editor exec vitest run lib/billing/__tests__/e2e — 3/3 pass
  • Run just lint-supabase-db and confirm no security_definer_view (lint 0010) findings remain
  • (Optional) Hit /organizations/<org>/settings/billing as an authenticated org member — same data shows as before
  • (Optional) Confirm in the Dashboard's Database Advisors page (after deploy) that the two v_billing_* warnings are gone

Out of scope / known noise

  • The pg_graphql_authenticated_table_exposed lint (different from 0010) still fires for the two views — by design. The views ARE intentionally discoverable by authenticated users; RLS filters which rows they see. Same lint fires for ~20 other public tables (organization, project, document, etc.) — repo-wide pattern, separate concern.
  • Pre-existing WARN-level findings (REMOVEME RLS policies in grida_forms, public bucket listing, etc.) are out of scope and predate billing.

Summary by CodeRabbit

  • New Features

    • Added a database lint command to run Supabase advisory checks against a local Postgres instance and surface filtered advisor warnings.
  • Chores

    • Tightened billing database access controls and view security to restrict public execution and enforce row-level scoping.
    • Updated database tests to validate the revised security and permission behavior.

Convert grida_billing's API surface to the canonical Supabase pattern:
security_invoker=true views over RLS-protected base tables, replacing
SECURITY DEFINER views over a fully-locked schema. Functionally equivalent
for every legitimate consumer; satisfies splinter lint 0010
(security_definer_view) which the Dashboard's Database Advisors flagged.

What changed:
- New migration 20260507223000_grida_billing_security_invoker.sql.
  The pushed 20260506132900 stays untouched.
- grida_billing schema now grants USAGE to authenticated + service_role
  (security_invoker views resolve `grida_billing.*` as the caller).
- account / subscription / audit drop their RESTRICTIVE deny-all policies
  and gain SELECT-only permissive policies (member_can_select for the
  first two, owner_can_select for audit). Writes stay service-role-only.
- product_catalogue / stripe_event remain fully locked — no API consumer
  reads them.
- v_billing_subscription / v_billing_audit flip to security_invoker=true.
  Their auth-side WHERE clauses move to the base-table policies; the
  views keep only the business filter (`status <> 'canceled'`).
- Both views explicitly REVOKE from PUBLIC, anon, authenticated and
  re-GRANT to authenticated + service_role. Closes Supabase's default
  ALL-on-public privilege creep that was making anon implicitly able
  to discover them via GraphQL introspection.

Behavioral equivalence (verified):
- Authenticated org members see the same rows pre/post (auth filter
  just moved from view body to RLS policy).
- Random users with no org → 0 rows, both before and after.
- Owners read v_billing_audit; non-owner members blocked, both before
  and after.
- anon: previously "implicit SELECT, returns 0 rows via NULL auth.uid()"
  → now "no SELECT grant, returns permission_denied". Strictly better
  posture; no legitimate flow affected.

Tests:
- pgTAP §13 (RLS denial contract) rewritten for the new model:
  10 anon-denial assertions + 3 authenticated-non-member-sees-zero
  + 2 locked-table denials + 5 INSERT denials = 20 total. Plan stays
  at 57; all tests pass.
- E2E billing suite (lifecycle, idempotency, tampered-signature)
  unchanged — webhook projector path untouched.
- pnpm --filter editor typecheck: clean.
- pnpm lint: 0 errors.
- 476 editor unit tests: pass.

Tooling:
- New `just lint-supabase-db` recipe runs Splinter advisors against
  the local DB. CLI doesn't ship a native `supabase db check` yet
  (supabase/cli#3839 open); inline curl + psql + awk suffices. Recipe
  links the upstream tracking issue + our comment so future maintainers
  can drop it when the CLI ships native support.

Sibling reading:
- supabase/cli#3839 — feature request for first-class CLI advisor command
  (we left a comment with our use case + recipe at #issuecomment-4397668907)
@vercel

vercel Bot commented May 7, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
blog Ready Ready Preview, Comment May 7, 2026 2:41pm
docs Ready Ready Preview, Comment May 7, 2026 2:41pm
grida Ready Ready Preview, Comment May 7, 2026 2:41pm
viewer Ready Ready Preview, Comment May 7, 2026 2:41pm
3 Skipped Deployments
Project Deployment Actions Updated (UTC)
backgrounds Ignored Ignored Preview May 7, 2026 2:41pm
code Ignored Ignored May 7, 2026 2:41pm
legacy Ignored Ignored May 7, 2026 2:41pm

Request Review

@coderabbitai

coderabbitai Bot commented May 7, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

The PR refactors the grida_billing schema security model from view-based row filtering to table-level RLS with security_invoker views, updates associated tests, tightens schema/function grants, and adds a just recipe to run Supabase splinter advisor locally.

Changes

Billing Schema Security Refactoring

Layer / File(s) Summary
Schema-level access control
supabase/schemas/grida_billing.sql, supabase/migrations/20260507223000_grida_billing_security_invoker.sql
Revokes PUBLIC schema USAGE and grants USAGE on grida_billing only to authenticated and service_role; revokes EXECUTE on functions from PUBLIC/anon/authenticated and grants EXECUTE only to service_role.
Table-level RLS policies
supabase/schemas/grida_billing.sql, supabase/migrations/20260507223000_grida_billing_security_invoker.sql
Replaces default-deny policies with member_can_select on account/subscription (org membership check) and owner_can_select on audit (org ownership check); adds index supporting owner lookup.
Public view security refactoring
supabase/schemas/grida_billing.sql, supabase/migrations/20260507223000_grida_billing_security_invoker.sql
Switches public.v_billing_subscription and public.v_billing_audit to WITH (security_invoker = true) and restricts SELECT grants to authenticated and service_role.
RLS/GRANT verification tests
supabase/tests/test_grida_billing_test.sql
Section 13 updated to validate anon denial and authenticated non-member RLS-filtered access; confirms non-members see zero rows and locked tables return permission errors (SQLSTATE 42501).

Database Linting Tooling

Layer / File(s) Summary
Lint recipe
justfile
Adds lint-supabase-db recipe that downloads Supabase splinter SQL, runs it read-only against a provided Postgres DSN, filters advisor output by configurable levels (default WARN,ERROR), formats findings, and deduplicates output.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested labels

org

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title accurately summarizes the main change: converting from SECURITY DEFINER views to security_invoker views with RLS on base tables, which is the core refactoring across all modified files.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refactor/billing-security-invoker-views

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 Microsoft Presidio Analyzer (2.2.362)
justfile

Microsoft Presidio Analyzer failed to scan this file


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 393588a508

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".


-- ─── 1. Schema USAGE ──────────────────────────────────────────────
REVOKE ALL ON SCHEMA grida_billing FROM PUBLIC;
GRANT USAGE ON SCHEMA grida_billing TO authenticated, service_role;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Revoke internal function execute rights before schema access

Granting authenticated USAGE on grida_billing makes every routine in that schema callable by authenticated users unless EXECUTE is explicitly revoked, and the existing billing migration defines multiple SECURITY DEFINER functions in grida_billing without corresponding REVOKE ... ON FUNCTION grida_billing.* FROM PUBLIC (only the public.fn_billing_* wrappers are locked down). This opens a privilege-escalation path where any signed-in user can invoke internal mutating functions (for example, event projector/customer attach paths) directly and bypass the intended service-role-only boundary.

Useful? React with 👍 / 👎.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (2)
justfile (1)

36-37: ⚡ Quick win

levels value is interpolated as an awk regex without sanitization.

{{replace(levels, ",", "|")}} is dropped verbatim into an awk ERE. Passing levels="WARN,ERROR|INFO" or any other ERE metacharacter (., *, [, () would silently widen or break the match. For a dev-only tool the blast radius is small, but a simple anchored string-comparison is safer and equally readable:

🔧 Proposed hardening (pure string match)
-      | awk -F'\t' -v lvls="^({{replace(levels, ",", "|")}})$" \
-          'NF >= 7 && $3 ~ lvls { print "  [" $3 "] " $1 ": " $7 }' \
+      | awk -F'\t' -v lvls=",{{levels}}," \
+          'NF >= 7 && index(lvls, "," $3 ",") { print "  [" $3 "] " $1 ": " $7 }' \
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@justfile` around lines 36 - 37, The awk invocation interpolates
{{replace(levels, ",", "|")}} into lvls and treats it as a regular expression,
allowing metacharacters in levels to widen or break matching; change to a pure
string comparison by escaping/sanitizing levels before interpolation or — better
— avoid EREs entirely and implement a string-equality check in the awk script
(e.g., split the levels string on commas and compare $3 against each element) so
lvls no longer contains raw regex metacharacters; update the command that
defines lvls and the awk condition that uses $3 to perform exact string matches
instead of $3 ~ lvls.
supabase/tests/test_grida_billing_test.sql (1)

453-470: ⚡ Quick win

Section 13 is missing positive-read assertions for the newly-opened tables.

The section thoroughly tests negative cases (0-row RLS filter for non-members, 42501 for locked tables and INSERTs). However, per the coding guideline, tests for tables that alter RLS should include both positive and negative assertions. There's no test confirming that an authenticated org member (e.g., alice) can successfully SELECT from grida_billing.account, grida_billing.subscription, and grida_billing.audit directly (the existing positive coverage in sections 9/10 is only through the public views).

✅ Suggested additions to section 13 (and update SELECT plan(N) accordingly)
-- ── authenticated org owner: positive direct-table reads ───────────────
SET LOCAL ROLE authenticated;
SELECT set_config('request.jwt.claim.sub', current_setting('test.alice_uid'), true);

SELECT ok(
  (SELECT count(*) FROM grida_billing.account WHERE organization_id = 2) > 0,
  'alice (owner) sees her org account row directly via RLS'
);
SELECT ok(
  (SELECT count(*) FROM grida_billing.subscription WHERE organization_id = 2) > 0,
  'alice (owner) sees her org subscription row directly via RLS'
);
SELECT ok(
  (SELECT count(*) FROM grida_billing.audit WHERE organization_id = 2) > 0,
  'alice (owner) sees her org audit row directly via RLS'
);
RESET ROLE;

As per coding guidelines: "Include both positive and negative assertions (ok(...), is(...), throws_ok(...))" and "Add pgTAP test coverage for every table that alters RLS, permissions, or tenant boundaries, including at minimum read isolation tests."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@supabase/tests/test_grida_billing_test.sql` around lines 453 - 470, Add
positive read assertions for authenticated org members by setting the JWT sub to
an owner (use SET LOCAL ROLE authenticated; SELECT
set_config('request.jwt.claim.sub', current_setting('test.alice_uid'), true);),
then add ok(...) tests that SELECT count(*) FROM grida_billing.account WHERE
organization_id = 2 > 0, from grida_billing.subscription WHERE organization_id =
2 > 0, and from grida_billing.audit WHERE organization_id = 2 > 0 to confirm
alice can directly read those tables via RLS, then RESET ROLE; ensure tests use
ok(...) and update the SELECT plan(N) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@justfile`:
- Around line 34-38: The recipe currently pipes psql into awk and sort so psql
failures are ignored; change it to capture psql output to a temporary file (use
mktemp), run the psql invocation (the `@psql` ... -f /tmp/splinter.sql ...
command) into that temp file, check its exit status and fail the recipe if
non‑zero, and only then run the awk ('awk -F'\t' -v lvls=... ...') and sort -u
steps reading from the temp file; ensure the temp file is removed on exit. This
preserves the existing awk/filter logic but guarantees psql errors cause the
recipe to fail.
- Line 33: The curl invocation that writes to the fixed path `/tmp/splinter.sql`
should be changed to download to a unique temporary file, verify its integrity,
and clean it up; update the URL to pin a commit SHA instead of `main`, create a
temp file (e.g., via mktemp), download there (the existing `curl -sSfL` usage),
compute and compare a provided checksum (sha256) before executing, and ensure
removal of the temp file on exit/failure (use a trap) so parallel CI jobs don’t
clobber each other and the SQL is integrity-checked.

In `@supabase/schemas/grida_billing.sql`:
- Around line 60-67: The RLS policies (e.g., member_can_select and
owner_can_select) reference public table join columns without indexes, causing
sequential scans during policy evaluation; add indexes for
public.organization_member.user_id and public.organization.owner_id (create
indexes named organization_member_user_id_idx and organization_owner_id_idx
respectively) so policy subqueries used by the account/subscription/audit
policies can use index lookups instead of sequential scans; ensure to use IF NOT
EXISTS semantics when adding these indexes so repeated migrations are safe.

---

Nitpick comments:
In `@justfile`:
- Around line 36-37: The awk invocation interpolates {{replace(levels, ",",
"|")}} into lvls and treats it as a regular expression, allowing metacharacters
in levels to widen or break matching; change to a pure string comparison by
escaping/sanitizing levels before interpolation or — better — avoid EREs
entirely and implement a string-equality check in the awk script (e.g., split
the levels string on commas and compare $3 against each element) so lvls no
longer contains raw regex metacharacters; update the command that defines lvls
and the awk condition that uses $3 to perform exact string matches instead of $3
~ lvls.

In `@supabase/tests/test_grida_billing_test.sql`:
- Around line 453-470: Add positive read assertions for authenticated org
members by setting the JWT sub to an owner (use SET LOCAL ROLE authenticated;
SELECT set_config('request.jwt.claim.sub', current_setting('test.alice_uid'),
true);), then add ok(...) tests that SELECT count(*) FROM grida_billing.account
WHERE organization_id = 2 > 0, from grida_billing.subscription WHERE
organization_id = 2 > 0, and from grida_billing.audit WHERE organization_id = 2
> 0 to confirm alice can directly read those tables via RLS, then RESET ROLE;
ensure tests use ok(...) and update the SELECT plan(N) accordingly.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 190f513c-5b97-4f92-a6b9-cbe523b53c16

📥 Commits

Reviewing files that changed from the base of the PR and between 41ce72d and 393588a.

📒 Files selected for processing (4)
  • justfile
  • supabase/migrations/20260507223000_grida_billing_security_invoker.sql
  • supabase/schemas/grida_billing.sql
  • supabase/tests/test_grida_billing_test.sql

Comment thread justfile Outdated
Comment thread justfile Outdated
Comment on lines +60 to +67
CREATE POLICY member_can_select ON grida_billing.account
FOR SELECT TO authenticated
USING (
organization_id IN (
SELECT om.organization_id FROM public.organization_member om
WHERE om.user_id = (SELECT auth.uid())
)
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify indexes exist on public.organization_member(user_id) and
# public.organization(owner_id) — the columns referenced in the new RLS policies.
rg -n --type sql -i 'create.*index.*organization_member.*user_id\|create.*index.*user_id.*organization_member' \
  || echo "No index on organization_member.user_id found"

rg -n --type sql -i 'create.*index.*organization.*owner_id\|create.*index.*owner_id.*organization[^_]' \
  || echo "No index on organization.owner_id found"

Repository: gridaco/grida

Length of output: 143


Add indexes on RLS join-key columns to avoid sequential scans during policy evaluation.

All three new SELECT policies subquery public tables without indexes on their join conditions:

Policy Subquery join column Status
member_can_select (account/subscription) public.organization_member.user_id Missing
owner_can_select (audit) public.organization.owner_id Missing

Each RLS-gated row scan will trigger a sequential table scan, once per candidate row. Under load this becomes a hot path.

Add these indexes to the public schema:

SQL Fix
CREATE INDEX IF NOT EXISTS organization_member_user_id_idx
  ON public.organization_member (user_id);

CREATE INDEX IF NOT EXISTS organization_owner_id_idx
  ON public.organization (owner_id);

Applies to lines: 60-67, 119-126, 261-268

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@supabase/schemas/grida_billing.sql` around lines 60 - 67, The RLS policies
(e.g., member_can_select and owner_can_select) reference public table join
columns without indexes, causing sequential scans during policy evaluation; add
indexes for public.organization_member.user_id and public.organization.owner_id
(create indexes named organization_member_user_id_idx and
organization_owner_id_idx respectively) so policy subqueries used by the
account/subscription/audit policies can use index lookups instead of sequential
scans; ensure to use IF NOT EXISTS semantics when adding these indexes so
repeated migrations are safe.

PR review fixes from #709. Directly amends the unmerged Pattern A
migration (`20260507223000_grida_billing_security_invoker.sql`) so
production gets one clean migration instead of a delta-on-delta.

Codex P1 — privilege escalation closed:
  Granting USAGE on `grida_billing` to authenticated (required for
  security_invoker views to resolve `grida_billing.*` as the caller)
  also made every internal SECURITY DEFINER routine reachable, because
  EXECUTE on functions defaults to PUBLIC and authenticated inherits
  PUBLIC. The original ALTER DEFAULT PRIVILEGES line only revoked from
  authenticated/anon, not PUBLIC, so authenticated could call the
  webhook projector (`fn_apply_stripe_event`) directly with arbitrary
  payloads and bypass signature verification.

  Fix: explicit `REVOKE EXECUTE ON ALL FUNCTIONS IN SCHEMA grida_billing
  FROM PUBLIC, anon, authenticated` for existing routines, and
  `ALTER DEFAULT PRIVILEGES … REVOKE EXECUTE … FROM PUBLIC` for future
  ones. Verified post-migration: only `postgres` and `service_role`
  retain EXECUTE on every grida_billing.fn_* and tg_*.

CodeRabbit #4 (partial) — RLS join-key index:
  `owner_can_select` on grida_billing.audit subqueries
  `public.organization` by `owner_id`, which has no index. Without it,
  every RLS evaluation on audit reads triggers a seq scan. Cheap fix.
  (CodeRabbit also flagged `organization_member.user_id`, but that
  index already exists — verified.)

CodeRabbit #2 — splinter download race + integrity:
  `/tmp/splinter.sql` collides under parallel CI matrix runs. Switched
  to `mktemp -t splinter.XXXXXX.sql` with `trap … EXIT` cleanup so
  concurrent invocations get isolated paths.

CodeRabbit #3 — pipeline error swallowing:
  `just` runs `/bin/sh` without pipefail; a `psql … | awk … | sort`
  failure used to exit 0 with empty output (no signal that linting
  never ran). Switched the recipe to a bash shebang with `set -euo
  pipefail` and capture-then-pipe, so connection/auth/schema-drift
  failures propagate. Verified: a bogus connection URL now exits
  non-zero with a clear psql error.

Verification:
- `supabase db reset` → both migrations apply clean
- `supabase test db` → 135 pgTAP tests pass
- `pnpm --filter editor typecheck` → clean
- `pnpm lint` → 0 errors
- `just lint-supabase-db` → no `security_definer_view` findings;
  the only remaining warnings are by-design GraphQL-discoverability
  notices on the two billing views (out of scope, see PR description)
- `pg_indexes` confirms `organization_owner_id_idx` is present
- `information_schema.routine_privileges` confirms only postgres +
  service_role hold EXECUTE on every grida_billing.fn_* / tg_*

E2E suite not re-run (requires `next dev` on :3000, which the sandbox
correctly refuses to autostart). Zero TS/projector code touched in
this commit, so the suite's pass/fail is unchanged from prior cycle.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
justfile (1)

40-40: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Pin splinter.sql to an immutable revision (and ideally verify checksum).

Fetching executable SQL from main makes lint results non-reproducible and weakens supply-chain trust. Prefer a commit-SHA URL and optional SHA-256 verification before psql.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@justfile` at line 40, Replace the unstable curl download of splinter.sql from
the branch with a pinned commit URL and add checksum verification: update the
curl target that writes to the $splinter variable to fetch the file using a
commit-SHA URL (not the /main path) and then verify its SHA-256 matches an
expected value before proceeding (e.g. compute the downloaded file's sha256 and
fail if it doesn't match); reference the existing $splinter variable and the
splinter.sql download step so the Justfile target aborts on checksum mismatch.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@justfile`:
- Line 43: The psql invocation passes the DSN/template variable unquoted which
can break for DSNs with spaces or shell-sensitive chars; update the command that
builds output (the psql call that currently uses {{url}}) to wrap the URL in
double quotes ("{{url}}") so the shell treats it as a single safe argument to
psql; ensure the change is applied where the output=$(psql {{url}} -X -qtA
-F$'\t' --pset=pager=off \) command is defined.

---

Duplicate comments:
In `@justfile`:
- Line 40: Replace the unstable curl download of splinter.sql from the branch
with a pinned commit URL and add checksum verification: update the curl target
that writes to the $splinter variable to fetch the file using a commit-SHA URL
(not the /main path) and then verify its SHA-256 matches an expected value
before proceeding (e.g. compute the downloaded file's sha256 and fail if it
doesn't match); reference the existing $splinter variable and the splinter.sql
download step so the Justfile target aborts on checksum mismatch.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d0ebaac7-17f2-40fa-bd8d-5f8059fcc51e

📥 Commits

Reviewing files that changed from the base of the PR and between 393588a and f1b70a6.

📒 Files selected for processing (3)
  • justfile
  • supabase/migrations/20260507223000_grida_billing_security_invoker.sql
  • supabase/schemas/grida_billing.sql
🚧 Files skipped from review as they are similar to previous changes (2)
  • supabase/schemas/grida_billing.sql
  • supabase/migrations/20260507223000_grida_billing_security_invoker.sql

Comment thread justfile
curl -sSfL https://raw.githubusercontent.com/supabase/splinter/main/splinter.sql -o "$splinter"
# Capture before filtering so any psql error propagates instead of being
# masked by an empty result set.
output=$(psql {{url}} -X -qtA -F$'\t' --pset=pager=off \

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/usr/bin/env bash
# Verify current interpolation is unquoted in recipe
rg -n 'output=\(psql \{\{url\}\}' justfile

Repository: gridaco/grida

Length of output: 39


🏁 Script executed:

# Read the justfile around line 43 to see the context
sed -n '40,50p' justfile

Repository: gridaco/grida

Length of output: 585


Quote the url argument in the psql call.

Line 43 currently passes {{url}} unquoted to psql. Change to "{{url}}" to safely handle DSNs containing shell-significant characters (spaces, wildcards, dollar signs, etc.).

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@justfile` at line 43, The psql invocation passes the DSN/template variable
unquoted which can break for DSNs with spaces or shell-sensitive chars; update
the command that builds output (the psql call that currently uses {{url}}) to
wrap the URL in double quotes ("{{url}}") so the shell treats it as a single
safe argument to psql; ensure the change is applied where the output=$(psql
{{url}} -X -qtA -F$'\t' --pset=pager=off \) command is defined.

@softmarshmallow softmarshmallow merged commit 33cc417 into main May 7, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant